-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(contentful-apps): Allow two project subpages to have same slug #16412
feat(contentful-apps): Allow two project subpages to have same slug #16412
Conversation
WalkthroughThe pull request introduces a new React functional component named Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
135-136
: Slugify user input to ensure consistent slug formatWhen the user manually edits the slug in the
TextInput
, the input is set directly without slugification. This could lead to invalid slugs with spaces or special characters. Consider slugifying the user's input to maintain a consistent and valid slug format.Apply this diff to slugify the user's input:
onChange={(ev) => { - setValue(ev.target.value) + setValue(slugify(ev.target.value)) }}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
1-10
: Imports and initial setup are appropriateThe imported modules and initial constants are correctly set up, adhering to best practices.
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx
Outdated
Show resolved
Hide resolved
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16412 +/- ##
==========================================
- Coverage 36.73% 36.69% -0.04%
==========================================
Files 6809 6809
Lines 141088 141061 -27
Branches 40225 40211 -14
==========================================
- Hits 51826 51764 -62
- Misses 89262 89297 +35
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 11 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
24-70
: LGTM: Well-structured useEffect hooks with a minor suggestionThe useEffect hooks for data fetching, listening to entry changes, and updating the slug based on title changes are well-implemented. They correctly use the CMA and SDK, and include appropriate checks for published entries and initial render.
A minor suggestion for improvement:
Consider extracting the logic inside the third useEffect (lines 48-66) into a separate function for better readability. For example:
const handleTitleChange = (newTitle: string) => { if (hasEntryBeenPublished || initialTitleChange.current) { initialTitleChange.current = false; return; } if (newTitle) { setValue(slugify(String(newTitle))); } }; useEffect(() => { return sdk.entry.fields.title .getForLocale(sdk.field.locale) .onValueChanged(handleTitleChange); }, [hasEntryBeenPublished, sdk.entry.fields.title, sdk.field.locale]);This change would improve the readability and maintainability of the component.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (4)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (4)
1-10
: LGTM: Imports and constants are well-organizedThe imports are appropriate for the component's functionality, and the debounce time constant is correctly defined at the top level. This follows good practices for React component organization.
11-23
: LGTM: Effective state management and initializationThe component's state management using useState hooks is well-structured and appropriate for its needs. The use of useRef to track the initial title change is a good approach to avoid unnecessary updates.
151-151
: LGTM: Correct component exportThe component is correctly exported as the default export, following common React practices.
80-80
:⚠️ Potential issueRemove console.log statement
The console.log statement on line 80 should be removed to prevent unwanted console output in production.
Apply this diff to remove the debug statement:
- console.log(projectPage)
Likely invalid or redundant comment.
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (4)
11-23
: LGTM: Well-structured component initialization with a suggestion for improvementThe component initialization and state management are well-implemented. The use of hooks for state management and SDK integration is appropriate, and initial states are correctly set up.
A suggestion for improvement:
Consider adding explicit type annotations to the state variables for better type safety and code readability. For example:const [value, setValue] = useState<string>(sdk.field?.getValue() ?? '') const [isValid, setIsValid] = useState<boolean>(true)This change would make the expected types more explicit and could help catch potential type-related issues earlier in development.
24-66
: LGTM: Well-implemented effect hooks with a suggestion for optimizationThe effect hooks for data fetching, entry changes, and title updates are well-implemented. They correctly handle various scenarios, including ignoring initial renders and respecting the publication status.
A suggestion for optimization:
Consider adding a cleanup function to the data fetching effect to cancel any ongoing requests if the component unmounts or the dependencies change. This can help prevent potential memory leaks or unnecessary API calls. For example:useEffect(() => { let isMounted = true; const fetchProjectPage = async () => { const response = await cma.entry.getMany({ // ... existing query ... }); if (isMounted && response.items.length > 0) { setProjectPage(response.items[0]); } }; fetchProjectPage(); return () => { isMounted = false; }; }, [cma.entry, sdk.entry]);This change ensures that the state is only updated if the component is still mounted, preventing potential issues with setting state on unmounted components.
72-124
: LGTM: Well-implemented validation logic and debounced updates with a suggestion for optimizationThe validation logic and debounced updates are well-implemented. The use of debounce for both validation and field updates is a good practice to prevent excessive API calls and improve performance.
A suggestion for optimization:
Consider memoizing thesubpageIds
array to avoid recalculating it on every validation check. You can use theuseMemo
hook for this:const subpageIds = useMemo(() => projectPage?.fields?.projectSubpages?.[defaultLocale]?.map( (subpage) => subpage.sys.id ) ?? [], [projectPage, defaultLocale] );Place this memoized value outside the debounced effect and use it within. This optimization would slightly improve performance, especially if the project has many subpages, by avoiding unnecessary recalculations.
126-147
: LGTM: Clean render logic with a suggestion for accessibility improvementThe render logic is well-implemented, using appropriate components from Contentful's design system. The conditional rendering of error messages and the TextInput's onChange handler are correctly implemented.
A suggestion for improving accessibility:
Consider addingaria-live
attributes to the error messages to ensure they are announced by screen readers when they appear. This would improve the component's accessibility. For example:- <Text fontColor="red400">Invalid slug</Text> + <Text fontColor="red400" aria-live="polite">Invalid slug</Text> - <Text fontColor="red400"> + <Text fontColor="red400" aria-live="polite"> Project subpage already exists with this slug </Text>This change would ensure that users relying on assistive technologies are aware of validation errors as they occur.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (1)
Pattern
apps/**/*
: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/contentful-apps/pages/fields/project-subpage-slug-field.tsx (2)
1-10
: LGTM: Well-structured imports and constant definitionThe imports are well-organized and include all necessary dependencies for the component's functionality. The
DEBOUNCE_TIME
constant is appropriately defined at the top level, enhancing maintainability.
149-149
: LGTM: Correct component exportThe component is correctly exported as the default export, following common React practices.
…16412) * Allow two subpages to have same slug * Change component name * Remove console.log --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Allow two project subpages to have same slug
What
Why
Example (both of these urls should be allowed):
/v/forsetakosningar-2024/fyrir-kjosendur
/v/althingiskosningar-2024/fyrir-kjosendur
Checklist:
Summary by CodeRabbit